perf: Avoid cloning signatures for simple port checks - #3149
Conversation
c8cef65 to
589cc3a
Compare
Merging this PR will improve performance by 9.63%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | simple_cfg |
383.3 µs | 328 µs | +16.87% |
| ⚡ | serialization/t_factory/capnp/with_extensions/encode |
11.7 ms | 10.2 ms | +14.35% |
| ⚡ | serialization/t_factory/capnp/without_extensions/encode |
10.8 ms | 9.6 ms | +12.25% |
| ⚡ | simple_dfg |
127.3 µs | 116.5 µs | +9.28% |
| ⚡ | fewnode_subgraph[1000] |
20 ms | 18.6 ms | +7.22% |
| ⚡ | serialization/simple_cfg/capnp/with_extensions/encode |
328.4 µs | 307.1 µs | +6.93% |
| ⚡ | serialization/t_factory/capnp/without_extensions/decode |
14.2 ms | 13.3 ms | +6.9% |
| ⚡ | fewnode_subgraph[100] |
2.1 ms | 2 ms | +6.84% |
| ⚡ | serialization/simple_cfg/capnp/without_extensions/encode |
309.5 µs | 290.5 µs | +6.57% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ab/envelope-perf-2 (238ba09) with main (e0540a1)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3149 +/- ##
==========================================
+ Coverage 81.55% 81.61% +0.06%
==========================================
Files 242 242
Lines 47373 47649 +276
Branches 40970 41246 +276
==========================================
+ Hits 38633 38888 +255
- Misses 6721 6737 +16
- Partials 2019 2024 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
589cc3a to
a544401
Compare
|
@codspeedbot explain why this is faster |
Why this PR is fasterCodSpeed measures a +14.79% overall improvement, driven by three benchmarks:
Root cause: eliminating whole-
|
| } | ||
|
|
||
| impl ValuePortOp for Call { | ||
| fn value_port_signature(&self) -> Option<&Signature> { |
There was a problem hiding this comment.
Does this mean that if you pass in the static-input port you'll get back None as the edge kind?
There was a problem hiding this comment.
Oh, sorry, that might be deliberate....
There was a problem hiding this comment.
Yes, this is trait replaces querying the signature for info on the value ports. Other ports are already tracked directly in the public OpTrait implementation.
Ideally the new ValuePortOp trait methods would be part of OpTrait, but I wanted to avoid breaking changes.
I can try unifying things more, or leave a TODO to seal OpTrait and add the methods there.
There was a problem hiding this comment.
I guess adding default-implemented methods to OpTrait wouldn't be a breaking change 🤔
acl-cqc
left a comment
There was a problem hiding this comment.
- Main concern here is just about duplicating logic in two places (the new method impls, and the old ones that constructs a Signature)
- Shame that this is internal to hugr-core; I don't see why external clients (e.g. some tket2 passes!) wouldn't benefit from the same
- Makes me think that we should make something like this be the main way to query a node - I think that means some mix of
- Deprecating
signature() - Changing the type
fn signature(&self) -> impl ValuePortThing(...maybe instead of the methods on the op) - Making
Signatureimplement this trait too, andfn signaturejust builds it as a cache of what the op methods are returning
- Deprecating
- Note that there is
HugrView::{in_,out_,}value_typesas well, that calls the "expensive" (sometimes) Signature...maybe they just need updating
a544401 to
110df42
Compare
|
Refactored the change, adding the new methods to |
| /// Returns the type of a value port. | ||
| /// | ||
| /// Implementations may override this to avoid constructing a complete | ||
| /// [`Signature`] when only one port type is needed. | ||
| fn value_port_type(&self, port: Port) -> Option<Type> { | ||
| self.dataflow_signature()?.port_type(port).cloned() | ||
| } | ||
|
|
||
| /// Returns the number of value ports in one direction. | ||
| /// | ||
| /// Implementations may override this to avoid constructing a complete | ||
| /// [`Signature`] when only its size is needed. | ||
| fn value_port_count(&self, dir: Direction) -> usize { | ||
| self.dataflow_signature() | ||
| .map_or(0, |signature| signature.port_count(dir)) | ||
| } |
There was a problem hiding this comment.
The new OpTrait methods are here.
They default to querying the signature so it's not a breaking change.
| let op = hugr.get_optype(n); | ||
| op.value_input_type(p).expect("must be dataflow edge") |
There was a problem hiding this comment.
I'm wondering if we should have a shorthand for this on HugrView.
There was a problem hiding this comment.
For get_optype(n).value_(in/out)put_type, for value_(in/out)put_type.expect, or all three? Not opposed to any of those
There was a problem hiding this comment.
Or (Node) -> impl Iterator<Item=Type>? (+direction, or *2 for in/out)
There was a problem hiding this comment.
Looking more into it, I think it's fine to leave the the port-specific type getters in OpType.
This PR already adds HugrView::value_types(node, dir) -> Iterator<(Port, Type)> and in_/out_ variants. That should be enough to simplify relevant calls.
There was a problem hiding this comment.
Hang on....HugrView already defines
hugr/hugr-core/src/hugr/views.rs
Line 496 in e609c66
Done, we're using the existing traits now
It's now in the public trait
Could be interesting, I'll track it in a separate issue since it would be a breaking change.
Updated! |
acl-cqc
left a comment
There was a problem hiding this comment.
Like this, good work @aborgna-q :)
Happy to approve like this but what do you think about having an iterator over (in/out-Port, Type) or even just Type for a node/direction? (value_ports only returns ports)
Could be on OpType instead of the method taking a in/out-Port, or as well, or I suppose on the Hugr(View)...
| let op = hugr.get_optype(n); | ||
| op.value_input_type(p).expect("must be dataflow edge") |
There was a problem hiding this comment.
For get_optype(n).value_(in/out)put_type, for value_(in/out)put_type.expect, or all three? Not opposed to any of those
| let op = hugr.get_optype(n); | ||
| op.value_input_type(p).expect("must be dataflow edge") |
There was a problem hiding this comment.
Or (Node) -> impl Iterator<Item=Type>? (+direction, or *2 for in/out)
| self.node_ports(node, dir) | ||
| .filter_map(move |port| sig.port_type(port).map(|typ| (port, typ.clone()))) | ||
| let op = self.get_optype(node); | ||
| op.value_ports(dir) |
There was a problem hiding this comment.
yeah, definitely feels like giving back types as well as ports would be good here
| Direction::Incoming => (&self.just_inputs, &self.rest), | ||
| Direction::Outgoing => (&self.just_outputs, &self.rest), | ||
| }; | ||
| head.get(port.index()) |
There was a problem hiding this comment.
consider chaining two and then .get on that?
There was a problem hiding this comment.
Chaining iterators produces a linear-time .get.
I rewrote this with simpler ifs.
|
|
||
| fn value_port_type(&self, port: Port) -> Option<Type> { | ||
| match port.direction() { | ||
| Direction::Incoming => self.variants.get(self.tag)?.get(port.index()).cloned(), |
There was a problem hiding this comment.
Just None if self.tag is out-of-bounds? In fn signature we panic (via expect)
There was a problem hiding this comment.
Updated, and gave it a better error message for all cases.
| #[case::tag(tag())] | ||
| #[case::tail_loop(tail_loop())] | ||
| #[case::conditional(conditional())] | ||
| fn value_ports_match_signature(#[case] op: OpType) { |
There was a problem hiding this comment.
Good test. You might want to consider call and dfg
0427210 to
256f3d1
Compare
256f3d1 to
3bb6709
Compare
acl-cqc
left a comment
There was a problem hiding this comment.
👍 Yah ok, agreed this is the best design that works 😆
| } | ||
|
|
||
| fn value_port_type(&self, port: Port) -> Option<Type> { | ||
| match port.direction() { |
There was a problem hiding this comment.
You might consider
if port.direction() == Direction::Incoming {
if port.index() == 0 {return Some(Type::function(.....))}
port = Port::new(Direction::Incoming, port.index() - 1); // harder than it should be?
}
self.signature.port_type(port)
| /// Return the TypeRow of the selected variant. | ||
| /// | ||
| /// Panics if the tag is out of bounds. | ||
| fn variant(&self) -> &TypeRow { |
There was a problem hiding this comment.
Maybe call this variant_row or similar to avoid any ambiguity
Avoids creating a whole signature object when calling
OpType::value_port_countandOpType::port_kind.Adds an internal
ValuePortOptrait to dispatch the call to each optype variant.Part of #1551